Conversation
897eb10 to
1fb8bc7
Compare
|
|
||
| maybe_log_event_buffer = | ||
| maybe_telemetry_processor = | ||
| if Config.enable_logs?() do |
There was a problem hiding this comment.
why not forwards all data?
There was a problem hiding this comment.
@dingsdax this is done in the subsequent PRs that are waiting in line :)
1fb8bc7 to
6577f75
Compare
6577f75 to
95975a9
Compare
95975a9 to
c081ca3
Compare
| defstruct [ | ||
| :buffers, | ||
| :priority_cycle, | ||
| :cycle_position, | ||
| :on_envelope, | ||
| :capacity, | ||
| :active_ref, | ||
| :active_item_count, | ||
| queue: :queue.new(), | ||
| size: 0 | ||
| ] |
There was a problem hiding this comment.
Bug: The Scheduler struct field active_item_count is not initialized, defaulting to nil. This can cause a crash when used in an arithmetic operation.
Severity: MEDIUM
Suggested Fix
Initialize active_item_count to 0 in the defstruct definition. This ensures the field always holds a valid integer, preventing potential ArithmeticError crashes when it's used in calculations.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: lib/sentry/telemetry/scheduler.ex#L46-L56
Potential issue: The `Scheduler` struct defines the `:active_item_count` field without a
default value, causing it to be initialized to `nil`. This field is later used in a
subtraction operation within the `handle_info/2` function when processing `:DOWN`
messages. If a `:DOWN` message is received before an envelope is processed and
`active_item_count` is set to an integer, the `Scheduler` process will crash with an
`ArithmeticError`. While the normal execution path may not trigger this, it's an unsafe
state that violates the field's typespec and can be triggered in edge cases or by future
code changes.
There was a problem hiding this comment.
Sorry for the late post merge review, and disclaimer: I have almost zero experience with Elixir, so take this with a grain of salt.
This is optional: if I'm not mistaken, the rate limits are only checked in the transport. You could already drop rate-limited items before adding them to the buffer. And you could drop rate-limited items in the scheduler before you pass them down to the transport. These points are optional, and I leave it up to you if it's worth the effort to add these.
The rest looks good to me, and thanks for adding this.
| defp offer(%Buffer{size: size, capacity: capacity} = state, item) | ||
| when size >= capacity do | ||
| {{:value, _dropped}, items} = :queue.out(state.items) | ||
| %{state | items: :queue.in(item, items)} | ||
| end |
There was a problem hiding this comment.
Follow up comment: if I'm not mistaken, this looks like we're dropping data here, and if we do, we must record client reports: https://develop.sentry.dev/sdk/foundations/processing/telemetry-processor/#telemetry-buffer
12.0.0releaseTelemetry Processorby defaultTelemetry Processorusing a new option calledtelemetry_processor_categories- I'll add those in separate PRs13.0.0we replace sender with telemetry processorSummary
Introduce the TelemetryProcessor — an OTP Supervisor that buffers, batches, and ships log events to Sentry asynchronously, implementing the Backend Telemetry Processor spec.
This is step 1 and currently only the
:logcategory is routed through it. Errors, transactions, and check-ins continue using existing transport paths.Architecture
spawn_monitoradd/2andflush/0Data flow
flowchart TD L["Logger"] --> LB["LogsBackend"] LB --> TP["TelemetryProcessor.add/2"] TP -->|cast| B["Buffer (1000 items)"] TP -->|cast| S["Scheduler"] S -->|poll| B B -->|batch| S S -->|spawn_monitor| HTTP["HTTP POST to Sentry"]